Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs/tech-notes: update "Statement Execution" and "Building execution plans" sections of "Life of a query" #60568

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

mneverov
Copy link
Contributor

docs/tech-notes: update "Statement Execution" and "Building execution plans" sections of "Life of a query"

Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Feb 14, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Feb 14, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan otan requested review from a team February 14, 2021 21:59
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks for the updates!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mneverov)


docs/tech-notes/life_of_a_query.md, line 189 at r1 (raw file):

children whose data it consumes).
                                
SQL query planning and optimizations are described in details in the 

[nit] in detail


docs/tech-notes/life_of_a_query.md, line 192 at r1 (raw file):

documentation for the [`opt` package](https://github.com/cockroachdb/cockroach/blob/9969862335cf501c1aa67a4aaf018ad5dc8e85e8/pkg/sql/opt/doc.go). 

In the meantime, the plan `Builder` [looks at the type of the

remove "in the meantime"
say the optbuilder instead of Builder (we have an execbuilder.Builder too)


docs/tech-notes/life_of_a_query.md, line 202 at r1 (raw file):

[`builder.buildScan()`](https://github.com/cockroachdb/cockroach/blob/9969862335cf501c1aa67a4aaf018ad5dc8e85e8/pkg/sql/opt/optbuilder/select.go#L114))
to scan a table, a `WHERE` clause is transformed into an [`memo.FilterExpr`](https://github.com/cockroachdb/cockroach/blob/9969862335cf501c1aa67a4aaf018ad5dc8e85e8/pkg/sql/opt/optbuilder/select.go#L1126),
an `ORDER BY` clause is [ordering physical property](https://github.com/cockroachdb/cockroach/blob/9969862335cf501c1aa67a4aaf018ad5dc8e85e8/pkg/sql/opt/optbuilder/orderby.go#L65),

[nit] is turned into


docs/tech-notes/life_of_a_query.md, line 208 at r1 (raw file):

Finally, the execution plan is simplified, [optimized](https://github.com/cockroachdb/cockroach/blob/5ee1f52f0eef1eeceddde16aad5002dc31ca08cb/pkg/sql/opt/xform/optimizer.go#L198), and
a physical plan is built.

I'd add a bit more detail about how the physical plan is built: the execbuilder generates execution operators through an exec.Factory (implemented by sql/opt_factory.go)

@blathers-crl
Copy link

blathers-crl bot commented Feb 15, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Copy link
Contributor Author

@mneverov mneverov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde)


docs/tech-notes/life_of_a_query.md, line 189 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] in detail

done


docs/tech-notes/life_of_a_query.md, line 192 at r1 (raw file):

Previously, RaduBerinde wrote…

remove "in the meantime"
say the optbuilder instead of Builder (we have an execbuilder.Builder too)

done


docs/tech-notes/life_of_a_query.md, line 202 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] is turned into

done


docs/tech-notes/life_of_a_query.md, line 208 at r1 (raw file):

Previously, RaduBerinde wrote…

I'd add a bit more detail about how the physical plan is built: the execbuilder generates execution operators through an exec.Factory (implemented by sql/opt_factory.go)

Added some info about physical plan building.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update! I have a couple of nits.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mneverov)


docs/tech-notes/life_of_a_query.md, line 157 at r2 (raw file):

documentation](https://www.cockroachlabs.com/docs/stable/transactions.html#client-side-intervention).
The statement execution code path continues inside the 
[`callback`](https://github.com/cockroachdb/cockroach/blob/9969862335cf501c1aa67a4aaf018ad5dc8e85e8/pkg/sql/conn_executor_prepare.go#L201), 

nit: I'm confused about this sentence and the link only adds to my confusion. I think I'd remove the sentence entirely.

Probably, originally "callback" referred to

txnClosure := func(txn *client.Txn, opt *client.TxnExecOptions) error {
, and I'm not sure what the equivalent to it now is, possibly it became obsolete / was refactored.


docs/tech-notes/life_of_a_query.md, line 188 at r2 (raw file):

right`](https://github.com/cockroachdb/cockroach/blob/33c18ad1bcdb37ed6ed428b7527148977a8c566a/pkg/sql/join.go#L125)
children whose data it consumes).
                                

super nit: something is off here (as viewing from Reviewable), seems like unnecessary space characters?

… plans" sections of "Life of a query"

Release note: None
Copy link
Contributor Author

@mneverov mneverov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)


docs/tech-notes/life_of_a_query.md, line 157 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I'm confused about this sentence and the link only adds to my confusion. I think I'd remove the sentence entirely.

Probably, originally "callback" referred to

txnClosure := func(txn *client.Txn, opt *client.TxnExecOptions) error {
, and I'm not sure what the equivalent to it now is, possibly it became obsolete / was refactored.

yes, you're right. The previous statement related to runTxnAttempt(...) in the callback that did run the statement. The closure I mentioned just creates a memo for a prepared statement, but actual execution is don in dispatchToExecutionEngine().
Updated the PR.


docs/tech-notes/life_of_a_query.md, line 188 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

super nit: something is off here (as viewing from Reviewable), seems like unnecessary space characters?

fixed, thx

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! :lgtm:

bors r=raduberinde,yuzefovich

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mneverov)


docs/tech-notes/life_of_a_query.md, line 157 at r2 (raw file):

Previously, mneverov (Max Neverov) wrote…

yes, you're right. The previous statement related to runTxnAttempt(...) in the callback that did run the statement. The closure I mentioned just creates a memo for a prepared statement, but actual execution is don in dispatchToExecutionEngine().
Updated the PR.

Thanks!

@craig
Copy link
Contributor

craig bot commented Feb 17, 2021

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Feb 17, 2021

Build succeeded:

@craig craig bot merged commit be115b9 into cockroachdb:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants